Skip to content

ci: stop the testcontainers reaper from deleting live test databases - #1235

Closed
NekoPunch (orangeCatDeveloper) wants to merge 1 commit into
agent-substrate:mainfrom
orangeCatDeveloper:fix/testcontainers-reaper-flake
Closed

ci: stop the testcontainers reaper from deleting live test databases#1235
NekoPunch (orangeCatDeveloper) wants to merge 1 commit into
agent-substrate:mainfrom
orangeCatDeveloper:fix/testcontainers-reaper-flake

Conversation

@orangeCatDeveloper

@orangeCatDeveloper NekoPunch (orangeCatDeveloper) commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Fixes #1230

cmd/ateapi/internal/controlapi fails ~30 tests at once in run-tests, every one on creating PostgreSQL test database: conn closed. A package's PostgreSQL container is deleted while that package is still using it.

Root cause

Every package binary under one go test ./... derives the same testcontainers session ID from the shared parent process, so all five packages that start a PostgreSQL container share one Ryuk reaper and one set of container labels. Ryuk deletes everything carrying that label once its connected-client count reaches zero.

The original failure had two upstream contributors:

  • Reusing an existing reaper waited only for its Docker port mapping. Docker exposes the port before Ryuk is listening, so the handshake could fail with read ack: EOF. testcontainers-go#3743 was fixed by #3761; testcontainers-go v0.44.0 is now in main via deps: bump testcontainers-go to v0.44.0 #1237.
  • Reaper.connect logs a failed handshake or startup and returns success anyway, so a package can run unregistered while its containers still carry the shared session label. #3827 remains open upstream. Under v0.44.0 the observed failure moves to wait for reaper <id>: context deadline exceeded rather than eliminating the shared-reaper race.

One package then exits, Ryuk sees zero clients, and prunes the session, including a database another package is mid-suite on. storetest's sync.Once blocks recreation, so every remaining test in that package fails on the same line.

The package count has since grown from four to five. That increases contention but does not change the defect.

Fix

Disable the reaper for run-tests.

The reaper exists to clean up after a process that dies without doing so. storetest and atepg now terminate their own containers, and a GitHub runner is destroyed with the job, so CI does not need a shared reaper. Local development is untouched and keeps it.

No test accompanies this: the failure lives in CI job composition and a dependency, neither of which is reachable from a Go test. Evidence is below instead.

Evidence

Ryuk's own log during a failing run; the container is removed, not crashed:

client disconnected  clients=0
prune check          clients=0
removed containers=1 networks=0 volumes=0 images=0

Forcing exactly one package to lose its reaper connection, with nothing else changed:

control   ok    cmd/ateapi/internal/store/atepg              10.229s
          ok    cmd/ateapi/internal/controlapi/functionaltest 30.673s

forced    ok    cmd/ateapi/internal/store/atepg              10.229s
          FAIL  cmd/ateapi/internal/controlapi/functionaltest 15.461s
                creating PostgreSQL test database: failed to connect to `user=postgres database=postgres`:
                  127.0.0.1:55110 (localhost): failed to receive message: read: connection reset by peer

30 of the 30 failing test names are among the 32 seen in run 32926359026.

Related changes


  • Tests pass
  • Appropriate changes to documentation are included in the PR

@orangeCatDeveloper

Copy link
Copy Markdown
Contributor Author

Split out the dependency bump into #1237 so this stays reviewable at four lines. The bump does not replace this change — see the cold-cache numbers there.

Tim Hockin (thockin) pushed a commit that referenced this pull request Aug 27, 2026
Part of #1230

When reusing an already-running Ryuk reaper, testcontainers-go v0.43.0
waits only for its Docker port mapping. Docker exposes that port before
Ryuk is listening, so a second package can connect too early and lose
the handshake with `read ack: EOF`. That is
[testcontainers-go#3743](testcontainers/testcontainers-go#3743);
v0.44.0 also waits for the reaper's `Started` log line
([#3761](testcontainers/testcontainers-go#3761)).

A package whose handshake fails is not counted as a Ryuk client but its
containers still carry the shared session label, so another package
exiting can delete a database that is still in use — the failure
reported in #1230.

## Scope

This helps local runs, where the reaper stays on. **It does not fix CI
on its own**, so it is deliberately separate from #1235, which disables
the reaper for `run-tests`.

Measured on a 4-core Linux VM with cold build caches, which staggers
package start times the way CI does:

```
v0.43.0, 3 rounds   handshake failures in 3/3 rounds; one round cascaded
v0.44.0, 3 rounds   no handshake failures; database tests still unavailable in 3/3 rounds
```

Under v0.44.0 the failures move to `wait for reaper <id>: context
deadline exceeded` — a late package finds a reaper that is already
shutting down, and one readiness probe (`defaultStartupTimeout`, 60s)
outlives the whole reaper retry budget (`MaxElapsedTime`, 20s), so the
retry loop never gets a second attempt. The remaining fail-open behind
all of this is
[#3827](testcontainers/testcontainers-go#3827),
still open upstream.

## Diff size

Four lines of `go.mod`. The rest is `go mod vendor` output: v0.44.0
pulls newer `moby/client`, `gopsutil`, and `otelhttp`, and `otelhttp`
moves `otel/semconv` from v1.39.0 to v1.41.0. Insertions and deletions
nearly cancel because most of it is a directory swap and one generated
`httpsnoop` file being merged into another.

```
go.mod, go.sum      62 lines
vendor/             61 files, 17356 +/17490 -
```

`hack/verify/go-modules.sh`, `licenses.sh`, `boilerplate.sh`, and
`gofmt.sh` all pass; `go test -race ./cmd/ateapi/...` is green with no
silently skipped database tests.

---

- [x] Tests pass
- [x] Appropriate changes to documentation are included in the PR
@BenTheElder

Copy link
Copy Markdown
Collaborator

Cost: the three packages that share storetest's fixture leave their container behind until the runner is torn down, because storetest has no termination hook. atepg terminates its own.

That seems unacceptable. Wherever possible CI should match local development behavior.

Leaking containers is a bug.

Comment on lines +32 to +33
# A lost reaper handshake gets a package's containers pruned mid-run
# (testcontainers-go#3827); a throwaway runner needs no reaping anyway.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should just fix the handshake. relying on "it's in CI" leads to bugs that cannot be tested locally.

@orangeCatDeveloper NekoPunch (orangeCatDeveloper) Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Locally the reaper stays on and the blast radius of #3827 is small — worst case a flaky full-suite run, and the fix for that is upstream (testcontainers-go#3841, not yet merged).

Disabling Ryuk in CI is also what the testcontainers docs recommend: "We recommend using it only for Continuous Integration services that have their own mechanism to clean up resources." — a GitHub-hosted runner is a fresh VM discarded after the job, so there's nothing left to reap.

`go test ./...` gives every package binary the same testcontainers session,
so they share one reaper, and it deletes that session's containers as soon
as its client count reaches zero -- including a database another package is
still using. A package whose handshake failed is not counted and has no way
to tell. Nothing here needs reaping: the runner is discarded with the job.

Claude-Session: https://claude.ai/code/session_01XuQqkwLf5Zx6CSZFHSC6hb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unit test race: cmd/ateapi/internal/controlapi

2 participants